Refuse a config field written with the wrong type - #564
Closed
alistair3149 wants to merge 2 commits into
Closed
Conversation
Fixes #560 config.json was parsed and then trusted. Its shape is declared as TypeScript interfaces, which are erased at runtime, so a value of the wrong type reached the code that reads it unchanged. Every boolean is read with a strict comparison, so a quoted "true" matched neither true nor false and the field fell back to its default: readOnly, private and allowWikiManagement all stayed open on a deployment the operator meant to lock down. The loader now checks every declared field of Config and WikiConfig against its type and refuses to start on a mismatch, naming the field and the type it got. A string where a boolean or a number belongs also gets "Remove the quotes.", since that is how the value comes to be wrong. null is accepted only where the declared type allows it, so readOnly: null stays refused as before and oauth2ClientId: null still round-trips. defaultWiki and wikis are covered too; both silently degraded to an empty value before. The field table is hand-rolled rather than a zod schema as the issue proposed. Validation here is interleaved with env-var substitution, exec-secret parsing and realpath resolution, which a schema cannot subsume, so zod would have left two validation systems and swapped operator-facing messages for generated ones. Considered, omitted: rejecting unknown keys, which would catch "readonly" written for readOnly but also refuse the "_comment" idiom; and requiring sitename, server, articlepath and scriptpath to be present, which is a separate contract from type and could refuse a config that works today. Verified by running the new tests against master: 9 of the 13 fail there, and the other 4 pin behaviour the field table must not relax. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two tables were typed Record<string, FieldType>, so nothing tied them to
Config and WikiConfig. A per-wiki boolean added later without a table entry
would have reopened the hole this validation closes, silently and with a green
typecheck, and a key typo'd as readonly would do it today. Keying the tables off
Exclude<keyof WikiConfig, SecretFieldName> makes both a compile error, verified
by removing an entry and by misspelling one.
Substitution turns out to be worth naming: it produces a string, so a boolean or
numeric field written as ${VAR} never took effect and is now refused. That is an
upgrade break for a config that used one, so it is called out in the changelog
and beside the substitution rules it contradicts.
An array reaching tags now reports "an array with a non-string entry" rather
than "an array", which read as a contradiction of the expectation beside it. The
comment on the WikiConfig cast no longer claims more than the check delivers:
the type of each field present is checked, its presence is not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #560
config.jsonwas parsed and then trusted. Its shape is declared as TypeScript interfaces, which are erased at runtime, so a value of the wrong type reached the code that reads it unchanged. Every boolean is read with a strict comparison, so a quoted"true"matched neithertruenorfalseand the field fell back to its default:readOnly,privateandallowWikiManagementall stayed open on a deployment the operator meant to lock down.The loader now checks every declared field of
ConfigandWikiConfigagainst its type and refuses to start on a mismatch, naming the field and the type it got. A string where a boolean or a number belongs also getsRemove the quotes., since that is the mistake.nullis accepted only where the declared type allows it, soreadOnly: nullstays refused as before andoauth2ClientId: nullstill round-trips.defaultWikiandwikisare covered too; both silently degraded to an empty value before.The two field tables are keyed off
Exclude<keyof WikiConfig, SecretFieldName>rather thanRecord<string, …>, so a field added to either interface without a table entry — or a table key typo'd asreadonly— is a compile error rather than a field that quietly stops being validated. Verified by removing an entry and by misspelling one.${VAR}substitution produces a string, so a boolean or numeric field written that way never took effect and is now refused. That is the one upgrade break here: a config with"oauth2CallbackPort": "${PORT}"booted before, with the port silently ephemeral, and now will not start. Called out in the changelog and beside the substitution rules it contradicts.The field table is hand-rolled rather than a zod schema as the issue proposed. Validation here is interleaved with env-var substitution, exec-secret parsing and
realpathresolution, which a schema cannot subsume, so zod would have left two validation systems and swapped operator-facing messages for generated ones.Considered, omitted
"readonly": truein a wiki entry still leaves that wiki writable, and that is a likelier operator mistake than quoting a boolean — the strongest argument for widening this PR. Held back because refusing an unknown key would break a config that works today, making it a breaking change rather than a fix, and a warning instead of a refusal is a design call worth making separately. Happy to add either here if you would rather it landed together.sitename,server,articlepathandscriptpathto be present. Presence is a separate contract from type, and enforcing it could refuse a config that works today. A wiki missingserverstill fails at first use asundefined/w/api.phprather than at startup.Verified by running the new tests against master: 9 of the 14 fail there, and the other 5 pin behaviour the field table must not relax. Each new test was also mutation-checked against 14 mutations of the validator — removing either validation call, flipping the nullable rule both ways, dropping the quoting hint, weakening the array check, and dropping each table entry — all caught.